Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Figure out CPU by MCU provided #50

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bmilanov
Copy link

Added a MCU_To_CPU function that returns the string representation
of the CPU based on the Device parameter. It is then used when generating
handlers.S

Added a MCU_To_CPU function that returns the string representation
of the CPU based on the Device parameter. It is then used when generating
handlers.S
@CLAassistant
Copy link

CLAassistant commented Sep 11, 2018

CLA assistant check
All committers have signed the CLA.

@simonjwright
Copy link
Contributor

Spello: Description_String, not Descrition_String

-- MCU_To_CPU --
-----------------

function MCU_To_CPU
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would need to declare the function first, at the beginning of the package. I'm a bit surprised this didn't raise a compilation error. I'll check the project file to make sure this rule is checked by gnat.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can put in place an automatic validation of the pull request with Travis-CI.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Also can confirm that the project builds and executes successfully even without the declaration.

Descrition_String : constant String := To_String (Device.Short_Desc);
CPU : Unbounded_String;
begin
if Starts_With (S1 => Descrition_String, S2 => "STM32F0") then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch is very useful, but I'm a bit uncomfortable with it: the element is supposed to be there for this purpose, while here we skip it entirely. So this makes svd2ada very dependant on ST-generated SVD files (and they don't seem to generate this optional information, while they should), while other vendors do and so we should first check if the information is present there.

Basically, I'm not refusing such consideration, it's only that to be fair, the SVD standard already provides means to provide this information, and having such routine that is ST-specific there without the more generic handling implemented first seems unfair, and risky (if ST decides at some point to fill the gaps).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that's cause for concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants